Skip to content

fix(renderer3d): populate Object3D.vertices for static batch (#630) - #664

Merged
aram-devdocs merged 3 commits into
mainfrom
codex/issue-630-static-model-invisible
Apr 3, 2026
Merged

fix(renderer3d): populate Object3D.vertices for static batch (#630)#664
aram-devdocs merged 3 commits into
mainfrom
codex/issue-630-static-model-invisible

Conversation

@aram-devdocs

@aram-devdocs aram-devdocs commented Apr 3, 2026

Copy link
Copy Markdown
Owner

Overview

Type: fix

Summary:
Fix SetModelStatic(true) making models invisible by populating Object3D.vertices for non-skinned models and instances. The static batch rebuild reads CPU-side vertex data from this field, but it was always Vec::new() for models (only primitives populated it correctly).

Related Issues: Fixes #630

Changes Made

Engine Core (goud_engine/src/)

  • libs/graphics/renderer3d/core_models/mod.rs: Populate Object3D.vertices with verts.clone() for non-skinned models (skinned models use 16 FPV layout incompatible with 8 FPV static batch)
  • libs/graphics/renderer3d/core_model_instances.rs: Clone vertices from source object when instantiating
  • libs/graphics/renderer3d/tests.rs: Add regression test verifying static primitives render via batch path

FFI Layer (goud_engine/src/ffi/)

No changes

C# SDK (sdks/csharp/)

No changes

Python SDK (sdks/python/)

No changes

TypeScript SDK (sdks/typescript/)

No changes

Codegen Pipeline (codegen/)

No changes

Proc Macros (goud_engine_macros/)

No changes

Tools (tools/)

No changes

WASM (goud_engine/src/wasm/)

No changes

Examples (examples/)

No changes

Documentation

No changes

Architectural Compliance

  • Rust-first: All logic lives in Rust; SDKs are thin wrappers
  • FFI boundary: No FFI changes
  • Dependency flow: Imports follow layer hierarchy (down only)
  • SDK parity: No FFI surface change
  • Unsafe discipline: No unsafe blocks added
  • File size: No file exceeds 500 lines

Testing

  • cargo test passes (pre-existing native_main_thread GPU test failure unrelated)
  • cargo clippy -- -D warnings is clean
  • cargo fmt --all -- --check passes
  • Python SDK tests pass — N/A (no SDK changes)
  • C# SDK tests pass — N/A (no SDK changes)
  • TypeScript SDK tests pass — N/A (no SDK changes)
  • Codegen produces consistent output — N/A (no codegen changes)
  • Pre-commit hooks pass

Code Quality

  • No todo!() or unimplemented!() in production code
  • No #[allow(unused)] without justification comment
  • Error handling uses Result, not unwrap()/expect() in library code
  • Public items have doc comments

Documentation

  • N/A — no architecture or user-facing behavior changes

Breaking Changes

None

Version Bump

Bump type: patch
Justification: Bug fix for static model rendering

Security

  • No new unsafe blocks
  • No new FFI pointer parameters
  • No new dependencies
  • No secrets or credentials

Performance

Non-skinned models now retain a CPU-side copy of vertex data in Object3D.vertices (same pattern primitives already use). This trades additional memory for correct static batching. For large scenes with many static models, the static batch reduces draw calls significantly — a net performance win at the cost of ~1x vertex memory per non-skinned model.

Deployment

  • N/A — engine-only change, no SDK version bumps needed

Reviewer Notes

Root cause: rebuild_static_batch() reads obj.vertices to bake world-space transforms into a single VBO. Models and instances were created with vertices: Vec::new(), producing zero vertices in the batch while being excluded from the dynamic render pass. Primitives worked because create_primitive() already populated this field.

aram-devdocs and others added 2 commits April 3, 2026 09:28
@aram-devdocs @claude
fix(renderer3d): populate Object3D.vertices for models and instances
4acfa0e 
Models and model instances were created with empty `vertices` fields,
causing the static batch rebuild to produce zero vertices while static
objects were excluded from the dynamic render pass -- making them
invisible when `SetModelStatic(true)` was called.

Primitives already populated this field correctly, which is why
`SetObjectStatic` on primitives worked. This fix applies the same
pattern to models and model instances (non-skinned only, since skinned
models use a 16-FPV layout incompatible with the 8-FPV static batch).

Fixes #630

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@aram-devdocs @claude
test(renderer3d): add regression test for static batch rendering (#630)
98a8b47 
Add test verifying that primitives marked static still render via the
static batch path. Also add clarifying comment on why skinned models
keep an empty vertices vec (16 FPV layout incompatible with 8 FPV
static batch format).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the size/S label Apr 3, 2026
@claude

claude Bot commented Apr 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Claude finished @aram-devdocs's task in 2m 9s —— View job

Code Review — fix(renderer3d): populate Object3D.vertices for static batch (#630)

  • Gather context and read CLAUDE.md
  • Read changed files
  • Phase 1: Context gathering
  • Phase 2: Architecture validation
  • Phase 3: Anti-pattern scan
  • Phase 4: Quality assessment
  • Phase 5: Security review (N/A — no FFI/unsafe)
  • Phase 6: Scope verification
  • Phase 7: Review output

Verdict: APPROVED ✅

The root cause diagnosis is accurate, the fix is minimal and correct. No blockers.

BLOCKERS

None.

WARNINGS

# File Line Issue
W1 tests.rs 386–415 Regression test targets primitives, which already worked before this PR
W2 tests.rs 407–410 Assertion draw_calls >= 1 is weak — doesn't verify the static batch produced geometry
W3 core_models/mod.rs 114 + 134 Non-skeletal models now clone verts twice: once into bind_pose_vertices (used only for CPU-skinned instancing) and once into Object3D.vertices

W1 detail: The PR description says primitives already worked because create_primitive() already populated vertices. The actual regression was for load_model and instantiate_model paths — but neither is covered by the new test. Loading a real model in a unit test requires constructing ModelData, which is admittedly heavier, but a direct unit test of Object3D.vertices being non-empty after load_model would be more targeted.

W2 detail: Since the cube is the only object in the test and it's marked static, a precise assertion would be assert_eq!(stats.draw_calls, 1). The current form (>= 1) would pass even if something unrelated produced a draw call.

W3 detail: In load_model, for non-skeletal models (has_skeleton = false, is_skinned = false):

bind_pose_vertices.push(verts.clone()); // line 114 — pre-existing, used only for CPU-skinned animation
// ...
vertices: if is_skinned { Vec::new() } else { verts.clone() }, // line 135 — new

bind_pose_vertices[i] for non-skeletal models is never accessed (the instancing path guards on has_skeleton, the animation path gather_skin_uploads is only called for skinned models). So for every non-skeletal static model there are two identical vertex copies in memory. This is a pre-existing waste amplified by the new clone. Not a blocker, but worth tracking in a follow-up.

Positive Callouts

  • Minimal and targeted fix. Only the two construction sites where vertices: Vec::new() were wrong are changed. No unnecessary refactoring.
  • Skinned vs non-skinned guard is correct. GPU-skinned models use 16 FPV (8 base + 4 bone indices + 4 weights), which is incompatible with the static batch's 8 FPV layout. Keeping Vec::new() for is_skinned prevents silent vertex misinterpretation.
  • Clear comment at the fix site (// CPU-side vertex copy for static batching (8 FPV layout). Skinned models use 16 FPV and are never static-batched.) makes the intent immediately legible.
  • Instance propagation in core_model_instances.rs correctly inherits the populated vertices from the source object via o.vertices.clone(), ensuring instances also work for static batching.
  • PR description is thorough — root cause, affected paths, and memory trade-off are all documented clearly.

@aram-devdocs @claude
fix(ci): increase Android build pipeline timeout to 25min
5e573a5 
Rust cross-compilation for Android takes ~12min without cache hits,
leaving insufficient time for both Gradle builds within the 15min
timeout. Matches the pattern from the iOS timeout fix (8a6ddd5).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/S and removed size/S labels Apr 3, 2026
@aram-devdocs
aram-devdocs merged commit f39f310 into main Apr 3, 2026
45 checks passed
@aram-devdocs aram-devdocs mentioned this pull request Apr 3, 2026
Merged
33 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

size/S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SetModelStatic(true) makes models invisible in 0.0.838

1 participant

@aram-devdocs